Add in progress note to internal_certificate_spec #34364
Conversation
| /// The cert-manager Issuer or ClusterIssuer to use for database internal communication. | ||
| /// The `issuerRef` field is required. | ||
| /// This currently is only used for environmentd, but will eventually support clusterd. | ||
| // Implementation in progress: |
There was a problem hiding this comment.
i think it makes sense to have the "implementation in progress" notes as a normal (non-doc) comment, but can we also add a big "do not use" warning as a doc comment here as well, so it shows up in our generated docs?
jubrad
force-pushed
the
materialize-cert-crd-doc-update
branch
from
e1a3252 to
21777ac
Compare
| /// The cert-manager Issuer or ClusterIssuer to use for database internal communication. | ||
| /// The `issuerRef` field is required. | ||
| /// This currently is only used for environmentd, but will eventually support clusterd. | ||
| /// Implementation in progress: |
There was a problem hiding this comment.
i don't think we need in progress notes in our public docs? can we just say "not yet implemented" in the public doc comment like console_external_certificate_spec does, and leave the progress notes as a non-doc comment?
jubrad
force-pushed
the
materialize-cert-crd-doc-update
branch
from
21777ac to
0d45755
Compare
jubrad
force-pushed
the
materialize-cert-crd-doc-update
branch
from
0d45755 to
d3bd887
Compare
| "name": "internalCertificateSpec", | ||
| "type": "MaterializeCertSpec", | ||
| "description": "The cert-manager Issuer or ClusterIssuer to use for database internal communication.\nThe `issuerRef` field is required.\nThis currently is only used for environmentd, but will eventually support clusterd.", | ||
| "description": "The cert-manager Issuer or ClusterIssuer to use for database internal communication.\nThe `issuerRef` field is required.\nThis currently is only used for environmentd, but will eventually support clusterd.\nNot yet implemented.", |
There was a problem hiding this comment.
Heh ... that "Not yet implemented" -- Is it referring to the clusterd part? Because it's in a standalone sentence ... it makes it sound like the whole thing is not yet implemented. Kind of like, if we had added a Deprecated afterwards ... it'd be more for the whole thing.
There was a problem hiding this comment.
Oh ... actually, reading the description... exactly, what is not implemented?
There was a problem hiding this comment.
using internalCertificateSpec will not do what people expect, because the implementation of that feature is only partially complete - customers should not be using it at all.
3 participants
Motivation
Currently we don't validate the cert on the balancer side. It'll still generated an used to encrypt but since there's no validation it's susceptible to MIM.
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.